This change seems to make the tests fail with SSHException: No hostkey for host localhost found. so we might probably need to update some tests too.

Hm, that's annoying, but makes sense. I'll update the tests either Thursday at the office or Saturday.

Wait, in previous code,

# https://github.com/apache/incubator-airflow/blob/f80138486e7828ff64fa544b4032a445b2ca140a/airflow/contrib/hooks/sftp_hook.py
if ignore_hostkey_verification == True => cnopts.hostkeys = None

# https://github.com/apache/incubator-airflow/blob/a0489d09fe4b0d3a2e0893c9cfba0e379e79390b/airflow/contrib/hooks/sftp_hook.py
# https://github.com/apache/incubator-airflow/blob/a0489d09fe4b0d3a2e0893c9cfba0e379e79390b/airflow/contrib/hooks/ssh_hook.py
if ignore_hostkey_verification == True => no_host_key_check = True  # inherit from SSHHook
if no_host_key_check == True => cnopts.hostkeys = None

Well, I got a little bit more confused, and after reading the code again I agree that @johnchenghk01 is right. The two options are not the inverse as I thought, it's just the default that is different. So the current code does indeed work because of SSHHook's default. Which does beg the question if SSHHook's default is sensible. And also, whether SFTPHook should follow that default. I think the last one is very important, because the default behaviour of 1.10 is to fail when the host key validation failed, but I think the changes of #3945 would allow them to succeed in 2.0. So now I think SFTPHook should change the default value of no_host_key_check to False and just set it to True when ignore_hostkey_validation == 'true'.

The default should be to validate SSH host keys.

Ok, I've updated the PR. By default the hook now validates the host keys, which is the same behaviour as 1.10. Note that this is different behaviour than the SSHHook, but it can be argued that the SSHHook is in the wrong there. The setting can be overridden using the deprecated ignore_hostkey_validation setting, or the no_host_key_check setting. I managed to get the tests running locally (after struggling a lot to set up an sftp server in my docker container) and I've added some tests to verify that all the settings now work as intended.

Codecov Report

Merging #4085 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4085   +/-   ##
=======================================
  Coverage   76.67%   76.67%           
=======================================
  Files         199      199           
  Lines       16186    16186           
=======================================
  Hits        12411    12411           
  Misses       3775     3775

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fc182e...2a7e641. Read the comment docs.